Reject port names containing whitespace by L4co77 · Pull Request #18 · PickNikRobotics/BehaviorTree.CPP

L4co77 · 2026-03-19T10:45:32Z

Add std::isspace check to IsAllowedPortName() to reject port names containing whitespace
Update CreatePort error message to mention the whitespace constraint
Add tests for whitespace rejection in port names

dsobek

Makes sense, just going to give this a test in against MoveIt Pro before approving.

For the forward port, could you evaluate if it makes sense to merge the upstream of behaviortree.cpp into our fork? In particular, I realized that some of the validation logic I pointed to you exists in the upstream master branch, and so there might be some duplicate work there. I created a separate issue for this https://github.com/PickNikRobotics/moveit_pro/issues/17640.

L4co77 · 2026-03-23T20:16:34Z

Makes sense, just going to give this a test in against MoveIt Pro before approving.

For the forward port, could you evaluate if it makes sense to merge the upstream of behaviortree.cpp into our fork? In particular, I realized that some of the validation logic I pointed to you exists in the upstream master branch, and so there might be some duplicate work there. I created a separate issue for this PickNikRobotics/moveit_pro#17640.

Commit 8f86eb9 adds comprehensive name validation that fully covers our whitespace check and more. Merging upstream into our fork would make this PR's BT.CPP change redundant. Should I take on #17640?

dsobek · 2026-03-23T21:20:10Z

Makes sense, just going to give this a test in against MoveIt Pro before approving.
For the forward port, could you evaluate if it makes sense to merge the upstream of behaviortree.cpp into our fork? In particular, I realized that some of the validation logic I pointed to you exists in the upstream master branch, and so there might be some duplicate work there. I created a separate issue for this PickNikRobotics/moveit_pro#17640.

Commit 8f86eb9 adds comprehensive name validation that fully covers our whitespace check and more. Merging upstream into our fork would make this PR's BT.CPP change redundant. Should I take on #17640?

If we still want to get this fixed for 8.10 I think we should apply this more minimal fix. Updating to the most recent state of the upstream behaviortree.cpp repo is a bit too risky of a change.
No rush on taking on #17640, I've loaded it onto the next iteration so that we can take care of it then. For the forward port, it might be nice to bundle this work together, but if it ends up being too much of a hassle, I think pointing main to the same behaviortree.cpp commit hash as we have in 8.10 (to include this change) is fine for the time being.

dsobek · 2026-03-23T23:43:02Z

src/xml_parsing.cpp

          {
            throw RuntimeError("Missing attribute [name] in port (SubTree model)");
          }
+          if(!IsAllowedPortName(name))


So this is turning out to be a bit strict. This is saying that _collapsed is not a valid port name, which is true, but it is a valid attribute.

BehaviorTree.CPP/src/xml_parsing.cpp

Lines 661 to 710 in c852132

for(const XMLAttribute* att = element->FirstAttribute(); att; att = att->Next())

{

const std::string port_name = att->Name();

std::string port_value = att->Value();

if(IsAllowedPortName(port_name))

{

if(port_value == "{=}")

{

port_value = StrCat("{", port_name, "}");

}

if(manifest)

{

auto port_model_it = manifest->ports.find(port_name);

if(port_model_it == manifest->ports.end())

{

throw RuntimeError(StrCat("a port with name [", port_name,

"] is found in the XML (<", element->Name(),

">, line ", std::to_string(att->GetLineNum()),

") but not in the providedPorts() of its "

"registered node type."));

}

const auto& port_model = port_model_it->second;

bool is_blacbkboard = port_value.size() >= 3 && port_value.front() == '{' &&

port_value.back() == '}';

// let's test already if conversion is possible

if(!is_blacbkboard && port_model.converter() && port_model.isStronglyTyped())

{

// This may throw

try

{

port_model.converter()(port_value);

}

catch(std::exception& ex)

{

auto msg =

StrCat("The port with name \"", port_name, "\" and value \"", port_value,

"\" can not be converted to ", port_model.typeName());

throw LogicError(msg);

}

}

}

port_remap[port_name] = port_value;

}

else if(!IsReservedAttribute(port_name))

{

other_attributes[port_name] = port_value;

}

}

is where ports are being parsed and this function is used there just to separate ports from non-ports.

This is the reason for the ci failure here (I printed the errors).

I also saw this when testing the moveit pro PR.

Maybe lets just move the std::any_of statement here and call it a day.

thanks, fixed

L4co77 requested a review from dsobek March 19, 2026 10:45

L4co77 self-assigned this Mar 19, 2026

L4co77 force-pushed the port-name-whitespace-validation branch from 6bf9c7a to a96649c Compare March 19, 2026 10:47

dsobek reviewed Mar 20, 2026

View reviewed changes

dsobek reviewed Mar 23, 2026

View reviewed changes

L4co77 force-pushed the port-name-whitespace-validation branch 2 times, most recently from 7a847c7 to 9080eff Compare March 24, 2026 08:49

Reject port names containing whitespace

56b976a

L4co77 force-pushed the port-name-whitespace-validation branch from 9080eff to 56b976a Compare March 24, 2026 08:51

L4co77 requested a review from dsobek March 24, 2026 10:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reject port names containing whitespace#18

Reject port names containing whitespace#18
L4co77 wants to merge 1 commit intomainfrom
port-name-whitespace-validation

L4co77 commented Mar 19, 2026 •

edited

Loading

Uh oh!

dsobek left a comment

Uh oh!

L4co77 commented Mar 23, 2026 •

edited

Loading

Uh oh!

dsobek commented Mar 23, 2026 •

edited

Loading

Uh oh!

dsobek Mar 23, 2026

Uh oh!

L4co77 Mar 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	for(const XMLAttribute* att = element->FirstAttribute(); att; att = att->Next())
	{
	const std::string port_name = att->Name();
	std::string port_value = att->Value();
	if(IsAllowedPortName(port_name))
	{
	if(port_value == "{=}")
	{
	port_value = StrCat("{", port_name, "}");
	}
	if(manifest)
	{
	auto port_model_it = manifest->ports.find(port_name);
	if(port_model_it == manifest->ports.end())
	{
	throw RuntimeError(StrCat("a port with name [", port_name,
	"] is found in the XML (<", element->Name(),
	">, line ", std::to_string(att->GetLineNum()),
	") but not in the providedPorts() of its "
	"registered node type."));
	}

	const auto& port_model = port_model_it->second;
	bool is_blacbkboard = port_value.size() >= 3 && port_value.front() == '{' &&
	port_value.back() == '}';
	// let's test already if conversion is possible
	if(!is_blacbkboard && port_model.converter() && port_model.isStronglyTyped())
	{
	// This may throw
	try
	{
	port_model.converter()(port_value);
	}
	catch(std::exception& ex)
	{
	auto msg =
	StrCat("The port with name \"", port_name, "\" and value \"", port_value,
	"\" can not be converted to ", port_model.typeName());
	throw LogicError(msg);
	}
	}
	}

	port_remap[port_name] = port_value;
	}
	else if(!IsReservedAttribute(port_name))
	{
	other_attributes[port_name] = port_value;
	}
	}

Conversation

L4co77 commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsobek left a comment

Choose a reason for hiding this comment

Uh oh!

L4co77 commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsobek commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsobek Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

L4co77 Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

L4co77 commented Mar 19, 2026 •

edited

Loading

L4co77 commented Mar 23, 2026 •

edited

Loading

dsobek commented Mar 23, 2026 •

edited

Loading